Skip to content

Update specterext-timelockrecovery to v0.2.3#2620

Open
oren-z0 wants to merge 1 commit into
cryptoadvance:masterfrom
oren-z0:upgrade-timelockrecovery-ext-0-2-3
Open

Update specterext-timelockrecovery to v0.2.3#2620
oren-z0 wants to merge 1 commit into
cryptoadvance:masterfrom
oren-z0:upgrade-timelockrecovery-ext-0-2-3

Conversation

@oren-z0
Copy link
Copy Markdown
Contributor

@oren-z0 oren-z0 commented May 9, 2026

Fixed floating point errors when converting BTC amount from BTC to sats (multiplying by 1e8).

Added descriptors to input of the recovery-transaction and the cancellation-transaction - necessary for some hardware wallets (i.e. Specter DIY) to identify the inputs.

Added descriptors to the self-sending outputs of the recovery-transaction and cancellation-transaction - will be displayed better on some hardware wallets (i.e. don't deduct from total-amount, show address-index and don't make it look like an external destination).

@netlify
Copy link
Copy Markdown

netlify Bot commented May 9, 2026

Deploy Preview for specter-desktop-docs canceled.

Name Link
🔨 Latest commit 84265fb
🔍 Latest deploy log https://app.netlify.com/projects/specter-desktop-docs/deploys/6a022cadc478250009407535

@al-munazzim
Copy link
Copy Markdown
Contributor

al-munazzim commented May 10, 2026

Review (security + bug-risk):

I checked this bump from specterext-timelockrecovery 0.2.1 -> 0.2.3 and the upstream delta (v0.2.1...v0.2.3).

What changed upstream:

  • float->sat conversion switched to Math.round(...) to avoid precision drift
  • PSBT input/output descriptor metadata is now filled explicitly (improves HW wallet signing/UX)
  • bech32 addresses normalized to lowercase

Security assessment:

  • No obvious new attack surface in this PR itself (only dependency pin update in requirements.in/requirements.txt).
  • Hash-pinned wheel/sdist entries are updated, which is good supply-chain hygiene.
  • The upstream changes are mostly transaction-construction correctness and metadata handling, not privilege/auth/network surface changes.

Potential bug/regression risks to watch:

  1. Rounding behavior changed (floor -> round) in multiple BTC->sat conversions; verify no +1 sat edge effects in split/leftover math.
  2. Descriptor-fill now raises on failure (SpecterError), so recovery/cancellation flows may fail hard where they previously progressed with less metadata.
  3. Auto-lowercasing bech32 is correct for bech32, but ensure no unintended mutation for non-bech32 recipient formats.

Suggested checks before merge:

  • Add/confirm test cases around BTC decimal edge values (e.g. 0.00098487, very small dust-adjacent values).
  • Exercise TimelockRecovery end-to-end with Specter DIY and at least one non-DIY signer.

Verdict: looks directionally good and addresses a real correctness bug; I’d merge after the above regression checks are green.

@al-munazzim
Copy link
Copy Markdown
Contributor

Thanks for the CI signal — I dug into the failing runs in detail.\n\nThe 3 red checks (, , ) are not caused by the extension version bump itself. All three fail early in the shared bootstrap step () with:\n\n- \n- \n\nSo the jobs fail before Python deps/tests for this PR’s code changes are exercised.\n\nI’ll prepare a separate PR to harden/fix this CI verification path. Once that is merged, we should be able to rerun this PR and get the checks green here as well.

@al-munazzim
Copy link
Copy Markdown
Contributor

Correction (my previous comment had formatting issues):

Thanks for the CI signal — I dug into the failing runs in detail.

The 3 red checks (test, extension-smoketest, cypress) are not caused by the extension version bump itself. All three fail early in the shared bootstrap step Install elementsd (tests/install_noded.sh --elements binary) with:

  • ERROR: upstream GPG verification failed for elements SHA256SUMS.asc
  • NO_PUBKEY 2F2A88D7F8D68E87

So the jobs fail before Python deps/tests for this PR’s code changes are exercised.

I’ll prepare a separate PR to harden/fix this CI verification path. Once that is merged, we should be able to rerun this PR and get the checks green here as well.

@al-munazzim
Copy link
Copy Markdown
Contributor

Follow-up is up: #2621\n\nIt hardens This script will result in having bitcoind or elementsd binaries, either by binary download or via compilation
Do one of these:
$ ./install_node.sh --bitcoin binary
$ ./install_node.sh --bitcoin compile
$ ./install_node.sh --elements binary
$ ./install_node.sh --elements compile
For more context, see https://github.com/cryptoadvance/specter-desktop/blob/master/docs/development.md#how-to-run-the-tests key import for elements signature verification (imports from both key sources before VALIDSIG check), which should address the failure path we saw.\n\nAfter #2621 merges, we can rerun this PR and checks should be able to go green.

@al-munazzim
Copy link
Copy Markdown
Contributor

Correction (previous comment had shell-formatting issues):

Follow-up is up: #2621

It hardens tests/install_noded.sh key import for elements signature verification (imports from both key sources before VALIDSIG check), which should address the Install elementsd failure path we saw.

After #2621 merges, we can rerun this PR and checks should be able to go green.

@k9ert k9ert force-pushed the upgrade-timelockrecovery-ext-0-2-3 branch from 09e9846 to 84265fb Compare May 11, 2026 19:23
@oren-z0
Copy link
Copy Markdown
Contributor Author

oren-z0 commented May 12, 2026

Rebased over @al-munazzim last commit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants